✨ Deprecate ClusterExtension spec.serviceAccount#2770
Merged
openshift-merge-bot[bot] merged 8 commits intoJul 20, 2026
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request deprecates ClusterExtension.spec.serviceAccount and removes the per-service-account authorization/impersonation infrastructure, shifting operator-controller to consistently use its own cluster-admin service account for cluster interactions. It also simplifies drift-detection/watch plumbing by moving from per-ClusterExtension cache management to a shared managedcache.TrackingCache, and updates manifests + test suites accordingly.
Changes:
- Deprecates
spec.serviceAccountin the API (godoc + schema changes) and adds aValidatingAdmissionPolicyto emit kubectl deprecation warnings when it’s set. - Removes per-SA authentication/authorization code paths, feature gates, and related tooling/dependencies.
- Updates Helm charts/manifests and E2E/unit tests to reflect the new model and add warning assertions.
Reviewed changes
Copilot reviewed 74 out of 77 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/extension-developer-e2e/extension_developer_test.go | Removes spec.serviceAccount usage from extension-developer E2E test. |
| test/e2e/steps/testdata/olm-sa-helm-rbac-template.yaml | Removes SA-scoped RBAC template used by older E2E flows. |
| test/e2e/steps/testdata/olm-sa-boxcutter-rbac-template.yaml | Removes SA-scoped Boxcutter RBAC template. |
| test/e2e/steps/testdata/olm-sa-boxcutter-no-create-rbac-template.yaml | Removes “no create verb” RBAC template previously used for preflight-permissions scenarios. |
| test/e2e/steps/testdata/namespace-template.yaml | Adds a reusable namespace YAML template for E2E setup. |
| test/e2e/steps/steps.go | Adds namespace/warning steps; captures kubectl stderr for warning assertions; removes preflight-permissions SA step. |
| test/e2e/steps/hooks.go | Tracks last kubectl apply stderr; removes removed feature gates from test feature map. |
| test/e2e/features/user-managed-fields.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/update.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/uninstall.feature | Drops SA setup/field and removes SA-deletion resilience scenario tied to old model. |
| test/e2e/features/status.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| test/e2e/features/revision.feature | Drops SA annotations and SA setup; adds namespace setup steps. |
| test/e2e/features/recover.feature | Drops SA setup/field and removes preflight-permissions recovery scenario. |
| test/e2e/features/proxy.feature | Drops SA setup/field; ensures namespace exists explicitly. |
| manifests/standard.yaml | Regenerates CRD schema (serviceAccount optional/deprecated), removes manager ClusterRole, binds operator-controller to cluster-admin, adds VAP + binding. |
| manifests/standard-e2e.yaml | Same as standard.yaml but for e2e manifest variant. |
| manifests/experimental.yaml | Same changes for experimental feature set manifest. |
| manifests/experimental-e2e.yaml | Same changes for experimental e2e manifest variant. |
| Makefile | Removes k8s-pin target and updates verify to use tidy. |
| internal/operator-controller/resolve/catalog_test.go | Updates tests to stop setting spec.serviceAccount. |
| internal/operator-controller/labels/labels.go | Removes SA annotation keys previously used to record service account identity. |
| internal/operator-controller/features/features.go | Removes PreflightPermissions and SyntheticPermissions feature gates. |
| internal/operator-controller/controllers/suite_test.go | Adds envtest setup for VAP + warning capture utilities. |
| internal/operator-controller/controllers/revision_engine_factory.go | Removes SA-scoped client creation; uses base config client for revision engines. |
| internal/operator-controller/controllers/clusterobjectset_controller_test.go | Updates controller tests to remove SA annotations/fields and SA-scoped error scenarios. |
| internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go | Updates internal controller tests to remove SA fields. |
| internal/operator-controller/controllers/clusterextension_reconcile_steps.go | Replaces SA existence validation with a deprecation log warning validator. |
| internal/operator-controller/controllers/clusterextension_controller.go | Adds a WithWatchesRawSource controller builder option. |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Removes SA-related setup and validator tests. |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Adds admission tests for deprecated spec.serviceAccount validation + VAP warning behavior. |
| internal/operator-controller/contentmanager/sourcerer.go | Removes old per-CE dynamic sourcerer implementation. |
| internal/operator-controller/contentmanager/source/internal/eventhandler.go | Removes copied controller-runtime event handler implementation. |
| internal/operator-controller/contentmanager/source/dynamicsource.go | Removes dynamic informer source implementation. |
| internal/operator-controller/contentmanager/source/dynamicsource_test.go | Removes tests for deleted dynamic source. |
| internal/operator-controller/contentmanager/contentmanager.go | Removes per-CE content manager and cache management layer. |
| internal/operator-controller/contentmanager/cache/cache.go | Removes managed-content cache implementation (watch lifecycle per CE). |
| internal/operator-controller/contentmanager/cache/cache_test.go | Removes tests for deleted cache implementation. |
| internal/operator-controller/authentication/tripper.go | Removes token-injecting round-tripper. |
| internal/operator-controller/authentication/tokengetter.go | Removes service-account token getter/cache. |
| internal/operator-controller/authentication/tokengetter_test.go | Removes tests for deleted token getter. |
| internal/operator-controller/authentication/synthetic.go | Removes synthetic impersonation utilities. |
| internal/operator-controller/authentication/synthetic_test.go | Removes tests for deleted synthetic impersonation utilities. |
| internal/operator-controller/applier/helm.go | Removes pre-auth; switches drift detection watch setup to shared tracking cache keyed by GVKs. |
| internal/operator-controller/applier/helm_test.go | Updates Helm applier tests to use mock tracking cache and drop pre-auth cases. |
| internal/operator-controller/applier/boxcutter.go | Removes SA annotations and pre-auth logic from Boxcutter path. |
| internal/operator-controller/applier/boxcutter_test.go | Updates Boxcutter tests to remove SA fields/annotations and pre-auth integration coverage. |
| internal/operator-controller/action/restconfig.go | Removes SA/synthetic rest config mappers. |
| internal/operator-controller/action/restconfig_test.go | Removes tests for deleted restconfig mappers. |
| helm/tilt.yaml | Removes PreflightPermissions from Tilt-enabled feature list. |
| helm/olmv1/values.yaml | Removes deprecated/removed feature gates from default values. |
| helm/olmv1/templates/validatingadmissionpolicybinding-clusterextension-serviceaccount-deprecated.yml | Adds chart template for VAP binding (Warn action). |
| helm/olmv1/templates/validatingadmissionpolicy-clusterextension-serviceaccount-deprecated.yml | Adds chart template for VAP that warns on deprecated serviceAccount usage. |
| helm/olmv1/templates/rbac/clusterrolebinding-operator-controller-manager-rolebinding.yml | Updates binding to reference cluster-admin. |
| helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml | Removes operator-controller manager ClusterRole template. |
| helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml | Regenerates standard CRD with deprecation + optional serviceAccount semantics. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml | Regenerates experimental CRD with deprecation + optional serviceAccount semantics. |
| helm/experimental.yaml | Removes removed feature gates from experimental values. |
| hack/tools/k8smaintainer/README.md | Removes docs for deleted k8s pinning tool. |
| hack/demo/synthetic-user-cluster-admin-demo-script.sh | Removes synthetic-user demo script tied to deleted features. |
| hack/demo/resources/synthetic-user-perms/cegroup-admin-binding.yaml | Removes synthetic-user demo RBAC. |
| hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml | Removes synthetic-user demo ClusterExtension example. |
| go.sum | Removes checksums tied to dropped dependencies (notably k8s.io/kubernetes). |
| go.mod | Drops k8s.io/kubernetes and replace directives; adjusts dependency direct/indirect classification. |
| docs/draft/howto/use-synthetic-permissions.md | Removes draft docs for synthetic permissions feature. |
| docs/api-reference/olmv1-api-reference.md | Updates generated API reference to reflect deprecation + optional serviceAccount. |
| cmd/operator-controller/main.go | Removes SA/synthetic/preflight wiring; introduces global tracking cache and raw watch source; binds to cluster-admin via charts/manifests. |
| applyconfigurations/api/v1/serviceaccountreference.go | Updates generated apply config docs to mark deprecated type/field. |
| applyconfigurations/api/v1/clusterextensionspec.go | Updates generated apply config docs to mark deprecated field and remove SA semantics. |
| api/v1/validation_test.go | Updates validation test defaults to no longer require serviceAccount. |
| api/v1/clusterextension_types.go | Deprecates spec.serviceAccount, switches it to optional omitzero, and relaxes name validation to allow empty. |
| .claude/TODO-OCPSTRAT-3040.md | Adds internal tracking checklist for the epic work items. |
| .claude/memory/update-ocpstrat-3040-todo.md | Adds a memory instruction to keep the TODO checklist updated. |
| .claude/memory/MEMORY.md | Adds index entry for the new memory note. |
Files not reviewed (2)
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2770 +/- ##
==========================================
+ Coverage 70.42% 71.60% +1.17%
==========================================
Files 143 132 -11
Lines 10625 9403 -1222
==========================================
- Hits 7483 6733 -750
+ Misses 2580 2180 -400
+ Partials 562 490 -72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dtfranz
reviewed
Jun 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 106 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
pedjak
commented
Jun 17, 2026
pedjak
commented
Jun 17, 2026
pedjak
commented
Jun 17, 2026
pedjak
marked this pull request as ready for review
June 17, 2026 10:41
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 103 out of 107 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Member
This might cause problems in systems that perform upgrades without using our upstream install scripts. Perhaps a safer alternative would be to continue using our existing role, but changing its rules to match what |
joelanford
reviewed
Jun 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comment on lines
+181
to
+183
| Initially, OLMv1 was designed as secure-by-default and required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. However, any users granted permission to create ClusterExtension resources are effectively being delegated cluster-admin trust. Requiring administrators to create least-privileged RBAC for ClusterExtension installation does not remove this avenue of privilege escalation. Thus, OLMv1 adopts cluster-admin scope directly to eliminate the complexity and UX burden of requiring users to manage ServiceAccounts with difficult-to-determine permissions, and enables simpler internal operations for namespace management and content lifecycle. | ||
|
|
||
| To make OLM secure by default: | ||
| Users who previously set `spec.serviceAccount` in their `ClusterExtension` manifests should remove that field. It is retained in the API for backwards compatibility — the API server will still accept it — but its value is silently ignored and will be removed in a future release. Because the operator-controller now always uses its own cluster-admin service account for all operations, cluster administrators should treat the ability to create or modify `ClusterExtension` resources as equivalent to granting cluster-admin privileges, and guard that access accordingly. To restrict which users may trigger installations, use Kubernetes RBAC on the `ClusterExtension` API; for finer-grained constraints such as limiting which packages, catalogs, namespaces, or upgrade policies a user may specify, pair RBAC with a `ValidatingAdmissionPolicy`. See [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md) for concrete examples of both approaches. |
Contributor
There was a problem hiding this comment.
Suggested change
| Initially, OLMv1 was designed as secure-by-default and required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. However, any users granted permission to create ClusterExtension resources are effectively being delegated cluster-admin trust. Requiring administrators to create least-privileged RBAC for ClusterExtension installation does not remove this avenue of privilege escalation. Thus, OLMv1 adopts cluster-admin scope directly to eliminate the complexity and UX burden of requiring users to manage ServiceAccounts with difficult-to-determine permissions, and enables simpler internal operations for namespace management and content lifecycle. | |
| To make OLM secure by default: | |
| Users who previously set `spec.serviceAccount` in their `ClusterExtension` manifests should remove that field. It is retained in the API for backwards compatibility — the API server will still accept it — but its value is silently ignored and will be removed in a future release. Because the operator-controller now always uses its own cluster-admin service account for all operations, cluster administrators should treat the ability to create or modify `ClusterExtension` resources as equivalent to granting cluster-admin privileges, and guard that access accordingly. To restrict which users may trigger installations, use Kubernetes RBAC on the `ClusterExtension` API; for finer-grained constraints such as limiting which packages, catalogs, namespaces, or upgrade policies a user may specify, pair RBAC with a `ValidatingAdmissionPolicy`. See [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md) for concrete examples of both approaches. | |
| OLMv1 adopts cluster-admin scope directly. Previously, OLM v1 required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. The current approach provides the following benefits: | |
| - Eliminates the complexity of difficult-to-determine permissions | |
| - Removes the UX burden of requiring users to manage ServiceAccounts | |
| - Enables simpler internal operations for namespace management and content lifecycle | |
| NOTE: | |
| The `spec.serviceAccount` field is depricated and will be removed in a future release. The field is retained in the API for backwards compatibility . The API server still accepts the field, but its value is silently ignored. | |
| For more information, see [Protecting OLMv1 API Access](../howto/how-to-protect-olmv1-api-access.md). |
That might be a little too aggressive, but feel free to take what is useful and disregard what is not.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 103 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comment on lines
+153
to
160
| if h.TrackingCache == nil { | ||
| return true, "", fmt.Errorf("TrackingCache not initialized, cannot set up drift detection watches") | ||
| } | ||
|
|
||
| if err := cache.Watch(ctx, h.Watcher, relObjects...); err != nil { | ||
| klog.FromContext(ctx).Info("watching managed objects") | ||
| gvks := gvksForObjects(relObjects) | ||
| if err := h.TrackingCache.Watch(ctx, ext, gvks); err != nil { | ||
| return true, "", err | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comment on lines
+408
to
+410
| // +kubebuilder:validation:XValidation:rule="self == oldSelf || size(self) == 0",message="name is immutable once set but may be cleared" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +required | ||
| Name string `json:"name"` | ||
| // +optional |
Contributor
There was a problem hiding this comment.
The enhancement proposal covers why we're not allowing "" - see here. The only modification that can be made to this field is completely removing spec.serviceAccount.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comment on lines
+169
to
+173
| for _, kind := range []string{"ValidatingAdmissionPolicy", "ValidatingAdmissionPolicyBinding"} { | ||
| objs, err := test.LoadManifests(kind) | ||
| utilruntime.Must(err) | ||
| for _, obj := range objs { | ||
| utilruntime.Must(cl.Create(ctx, obj)) |
joelanford
reviewed
Jul 14, 2026
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable once set but may be cleared" |
Member
There was a problem hiding this comment.
I'm noticing that once I clear the field, I'm able to set a serviceAccount.name on it. I'm guessing self == oldSelf is not catching that case because it only fires when there is an oldSelf to compare against.
Member
There was a problem hiding this comment.
I realized the absent -> set case may be impossible to block since we need to keep allowing the initial create to set a service account. CEL cannot distinguish between these:
- initial create with
.spec.serviceAccount.nameset - update from absent
.spec.serviceAccountto.spec.serviceAccount.nameset`
Contributor
There was a problem hiding this comment.
Couldn't we do this with another VAP? We could remove UPDATE from the current VAP's operations and add another VAP for it which gives errors instead of warnings.
joelanford
reviewed
Jul 14, 2026
| ### Secure access to ClusterExtensions through API access RBAC and ValidatingAdmissionPolicy | ||
|
|
||
| OLMv0 runs as cluster-admin, which is a security concern. OLMv0 has optional security controls for operator installations via the OperatorGroup, which allows a user with permission to create or update them to also set a ServiceAccount that will be used for authorization purposes on operator installations and upgrades in that namespace. If a ServiceAccount is not explicitly specified, OLM’s cluster-admin credentials are used. Another avenue that cluster administrators have is to lock down permissions and usage of the CatalogSource API, disable default catalogs, and provide tenants with custom vetted catalogs. However if a cluster admin is not aware of these options, the default configuration of a cluster results in users with permission to create a Subscription in namespaces that contain an OperatorGroup effectively have cluster-admin, because OLMv0 has unlimited permissions to install any bundle available in the default catalogs and the default community catalog is not vetted for limited RBAC. Because OLMv0 is used to install more RBAC and run arbitrary workloads, there are numerous potential vectors that attackers could exploit. While there are no known exploits and there has not been any specific concern reported from customers, we believe CNCF’s reputation rest on secure cloud-native software and that this is a non-negotiable area to improve. | ||
| OLMv1 adopts cluster-admin scope directly. Previously, OLMv1 required cluster administrators to create ServiceAccounts and associated RBAC to enable the installation of ClusterExtensions. The current approach provides the following benefits: |
Member
There was a problem hiding this comment.
I would suggest moving the Previously... sentence into the field deprecation note.
And then I think the next sentence and bullet points are still sort of comparing/contrasting with the previous logic, which again I think is only relevant in the deprecation note (if at all).
I'll try to wordsmith more and use only present tense descriptions and motivations.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
tmshort
reviewed
Jul 14, 2026
Comment on lines
-260
to
-281
|
|
||
| replace k8s.io/api => k8s.io/api v0.36.1 | ||
|
|
||
| replace k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.36.1 | ||
|
|
||
| replace k8s.io/apimachinery => k8s.io/apimachinery v0.36.1 | ||
|
|
||
| replace k8s.io/apiserver => k8s.io/apiserver v0.36.1 | ||
|
|
||
| replace k8s.io/cli-runtime => k8s.io/cli-runtime v0.36.1 | ||
|
|
||
| replace k8s.io/client-go => k8s.io/client-go v0.36.1 | ||
|
|
||
| replace k8s.io/component-base => k8s.io/component-base v0.36.1 | ||
|
|
||
| replace k8s.io/component-helpers => k8s.io/component-helpers v0.36.1 | ||
|
|
||
| replace k8s.io/controller-manager => k8s.io/controller-manager v0.36.1 | ||
|
|
||
| replace k8s.io/kubectl => k8s.io/kubectl v0.36.1 | ||
|
|
||
| replace k8s.io/streaming => k8s.io/streaming v0.36.1 |
Contributor
There was a problem hiding this comment.
Are these replaces no longer needed?
Contributor
There was a problem hiding this comment.
Yes - this copilot comment highlighted it as well.
Member
|
/approve |
perdasilva
reviewed
Jul 17, 2026
perdasilva
left a comment
perdasilva
left a comment
Contributor
There was a problem hiding this comment.
Finding — semantic mismatch on finalizer constant (not inline-commentable; line 60 is outside the diff hunks):
internal/operator-controller/controllers/clusterextension_controller.go:60
The constant ClusterExtensionCleanupContentManagerCacheFinalizer still holds the string "olm.operatorframework.io/cleanup-contentmanager-cache", but the finalizer handler now calls trackingCache.Free (Helm path) or is a no-op (Boxcutter path). Neither path involves a content manager.
This string is baked into etcd on all existing ClusterExtension objects. Operators debugging stuck finalizers and future maintainers will search for content-manager cleanup logic that no longer exists. Consider renaming the constant and updating the string (with a migration step for existing objects), or adding a comment explaining the historical name.
Mark spec.serviceAccount as deprecated in the ClusterExtension API. Remove PreAuthorizer integration, SA-scoped RBAC templates, and restconfig action. Simplify the controller to use cluster-admin scope directly. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
…rning Create VAP and VAPB resources that emit a warning when spec.serviceAccount is set on a ClusterExtension. Add integration test infrastructure for warning collection. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Contributor
Author
/hold cancel openshift/enhancements#2054 got merged recently. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comments suppressed due to low confidence (2)
api/v1/clusterextension_types.go:412
- The XValidation rule for
ServiceAccountReference.namestill enforces strict immutability (self == oldSelf) even though the message says the field “may be cleared”. If the field is removed during an UPDATE (clearing),selfcan become unset and this rule may reject the update, contradicting the intended “allow clearing” behavior described in the PR.
Consider updating the rule to explicitly allow an unset self (and allow first-time set), e.g. !has(oldSelf) || !has(self) || self == oldSelf, then regenerate CRDs/manifests.
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=253
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable once set but may be cleared"
// +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters"
// +optional
Name string `json:"name,omitempty"`
internal/operator-controller/controllers/clusterextension_reconcile_steps.go:106
ServiceAccountDeprecationWarninglogs at info level on every reconciliation as long asspec.serviceAccount.nameis set. This can generate noisy logs (and potentially high volume) for existing users until they remove the deprecated field.
Consider lowering the verbosity (e.g., l.V(1).Info(...)) and/or gating the log to only happen when the spec generation changes so it behaves like a warning without spamming.
Contributor
|
/lgtm |
Contributor
Author
|
/hold |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 104 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comments suppressed due to low confidence (1)
api/v1/clusterextension_types.go:410
- The XValidation rule for ServiceAccountReference.name still enforces strict immutability (
self == oldSelf). WithNamenow optional, this will reject attempts to clear the field (new value becomes null/absent) and also rejects creates unless the apiserver treatsoldSelfspecially. This contradicts the updated message (“may be cleared”) and the PR goal of allowing the deprecated field to be removed.
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=253
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable once set but may be cleared"
// +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters"
Remove SA-specific e2e test scenarios, SA RBAC templates, and the extension developer test suite. Simplify test steps to use cluster-admin scope. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Delete authentication and authorization packages, feature gates, k8smaintainer tool, SA demo scripts, and documentation. Remove k8s.io/kubernetes dependency and unnecessary k8s staging replace directives. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Delete the contentmanager package and replace it with boxcutter's shared TrackingCache. Simplify revision engine factory to use the base config directly. Remove SA label constants. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
Remove SA-related documentation, tutorials, and demo scripts. Clean up remaining ServiceAccount references from unit tests. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com>
…tect OLMv1 API access. Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-Authored-By: Predrag Knezevic <pknezevi@redhat.com> Co-Authored-By: Daniel Franz <dfranz@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Also updates go.mod/go.sum to fix k8s.io/client-go version.
Contributor
Author
|
/hold cancel |
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, rashmigottipati The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 101 out of 105 changed files in this pull request and generated 2 comments.
Files not reviewed (3)
- applyconfigurations/api/v1/clusterextensioninstallconfig.go: Generated file
- applyconfigurations/api/v1/clusterextensionspec.go: Generated file
- applyconfigurations/api/v1/serviceaccountreference.go: Generated file
Comments suppressed due to low confidence (2)
internal/operator-controller/applier/helm.go:1
- This now silently succeeds (returns
nilerror) even when drift detection setup fails due to manifest parsing or cache/watch errors. That can lead to “healthy looking” installs with drift detection unexpectedly disabled, which is hard to detect operationally. Consider propagating an error (so the reconcile retries), or at least surfacing this as an explicit condition/event on the owning ClusterExtension so operators can detect that drift detection is not active.
internal/operator-controller/controllers/suite_test.go:1 - Creating these admission resources unconditionally will fail the test suite if they already exist (e.g., when the apiserver state is reused, or if multiple packages seed the same objects). To make the suite setup idempotent, consider using
CreatewithIgnoreAlreadyExists, or switching to server-side apply / patch semantics so repeated TestMain runs don’t hard-fail during setup.
Comment on lines
+69
to
+77
| // serviceAccount is a deprecated field and is completely ignored. | ||
| // OLMv1 is a single-tenant system where users with ClusterExtension write access are | ||
| // effectively delegated cluster-admin trust. The operator-controller runs with | ||
| // cluster-admin privileges and uses its own service account for all cluster interactions. | ||
| // | ||
| // +required | ||
| ServiceAccount ServiceAccountReference `json:"serviceAccount"` | ||
| // Deprecated: serviceAccount is no longer used and will be removed in a future release. | ||
| // | ||
| // +optional | ||
| ServiceAccount ServiceAccountReference `json:"serviceAccount,omitzero"` |
Comment on lines
+407
to
+412
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable once set but may be cleared" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +required | ||
| Name string `json:"name"` | ||
| // +optional | ||
| Name string `json:"name,omitempty"` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Deprecate
spec.serviceAccounton ClusterExtension. OLMv1 is a single-tenant system where users with ClusterExtension write access are effectively delegated cluster-admin trust. With the service account field deprecated, operator-controller uses its own cluster-admin service account for all cluster interactions, eliminating the per-SA permission model and ~5,700 lines of supporting infrastructure.Changes
API deprecation:
spec.serviceAccountas deprecated withDeprecated:godoc convention andomitzerotagserviceAccountis setserviceAccount.nameimmutability rule to allow clearing the deprecated fieldRemove per-SA infrastructure:
authorization/package,authentication/package, SA-scoped config mappersk8s.io/kubernetesdependency and transitive replace directives from go.modhack/tools/k8smaintainer/andk8s-pinMakefile targetPreflightPermissionsandSyntheticPermissionsfeature gatesSimplify cache architecture:
contentmanager/package with sharedTrackingCachefrom boxcutter/managedcacheRestConfigMapperpatternE2E test cleanup:
serviceAccountfrom all feature files, add explicit namespace setup stepUpgrade safety:
Removed ~5,700 lines of unneeded code.
Reviewer Checklist